rm: introduce manager write roles and gates #10227
Conversation
Signed-off-by: okjiang <819421878@qq.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds a role-based write permission model (ResourceGroupWriteRole) and enforces it across manager, keyspace, and service-limiter logic; updates APIs and tests to propagate and handle write-disabled errors and to optionally persist service-limit changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as APIServer
participant Mgr as Manager
participant KSM as KeyspaceRGManager
participant Store as Storage
Client->>APIServer: POST /setKeyspaceServiceLimit(keyspaceID, limit)
APIServer->>Mgr: SetKeyspaceServiceLimit(keyspaceID, limit)
Mgr->>KSM: updateServiceLimit(limit, persistBasedOnWriteRole)
alt writeRole AllowsMetadataWrite
KSM->>Store: persist service limit
Store-->>KSM: OK
else metadata write disabled
KSM-->>Mgr: errMetadataWriteDisabled
end
KSM-->>Mgr: result (ok / error)
Mgr-->>APIServer: (nil / error)
alt success
APIServer-->>Client: 200 "Success!"
else metadata write disabled
APIServer-->>Client: 403 error message
else other error
APIServer-->>Client: 500 error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
bot
added
size/XL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10227 +/- ##
==========================================
+ Coverage 78.63% 78.66% +0.03%
==========================================
Files 520 521 +1
Lines 70089 70148 +59
==========================================
+ Hits 55112 55183 +71
- Misses 10989 10994 +5
+ Partials 3988 3971 -17
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/mcs/resourcemanager/server/apis/v1/api.go`:
- Around line 378-381: The handler currently returns http.StatusBadRequest for
any error from s.manager.SetKeyspaceServiceLimit which is incorrect for
server-side role-gating (errMetadataWriteDisabled) and inconsistent with other
handlers; update the error handling in the SetKeyspaceServiceLimit call so that
if the returned error is the sentinel errMetadataWriteDisabled you respond with
http.StatusForbidden (403) and for other manager errors respond with
http.StatusInternalServerError (500) to match
postResourceGroup/putResourceGroup/deleteResourceGroup behavior; locate the call
to s.manager.SetKeyspaceServiceLimit and adjust the conditional to inspect the
error value and map it to 403 for errMetadataWriteDisabled, otherwise return 500
with the error message.
🧹 Nitpick comments (4)
pkg/mcs/resourcemanager/server/write_role.go (1)
41-41: Nit: consider addingerrStateWriteDisabledfor symmetry.Currently, state write gating silently skips persistence with no error propagation. This is fine if by design state writes should fail silently, but if an
errStateWriteDisabledsentinel is ever needed for future APIs or diagnostics, it could be added here. Not blocking.pkg/mcs/resourcemanager/server/service_limit.go (1)
51-60: Consider using a required parameter instead of variadic forwriteRole.The variadic
writeRoles ...ResourceGroupWriteRolesilently discards all but the first element if multiple are passed. All current call sites pass exactly one value. A required parameter would be clearer and avoid this ambiguity.Suggested signature
-func newServiceLimiter( - keyspaceID uint32, - serviceLimit float64, - storage endpoint.ResourceGroupStorage, - writeRoles ...ResourceGroupWriteRole, -) *serviceLimiter { - writeRole := ResourceGroupWriteRoleLegacyAll - if len(writeRoles) > 0 { - writeRole = writeRoles[0] - } +func newServiceLimiter( + keyspaceID uint32, + serviceLimit float64, + storage endpoint.ResourceGroupStorage, + writeRole ResourceGroupWriteRole, +) *serviceLimiter {pkg/mcs/resourcemanager/server/keyspace_manager.go (2)
77-93: Same variadic concern asnewServiceLimiter.Same suggestion as in
service_limit.go— consider a required parameter forwriteRolesince all call sites pass exactly one value.
195-213:deleteResourceGroupproceeds with in-memory deletion even when metadata write is skipped.When
AllowsMetadataWrite()is false (e.g.,RMTokenOnly), Lines 205–209 skip the storage deletion but Lines 210–212 still remove the group from the in-memory map. On restart, the group would reappear from storage settings. This is safe in practice becauseManager.DeleteResourceGroupalready gates at Line 372, so this code path is unreachable with a non-metadata role. However, for defense-in-depth consistency, consider returning early (similar topersistResourceGroupRunningState) rather than silently diverging memory from storage.
| @@ -0,0 +1,41 @@ | |||
| // Copyright 2025 TiKV Project Authors. | |||
There was a problem hiding this comment.
| // Copyright 2025 TiKV Project Authors. | |
| // Copyright 2026 TiKV Project Authors. |
Signed-off-by: okjiang <819421878@qq.com>
|
/retest |
1 similar comment
|
/retest |
|
@okJiang: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/cc @rleungx @bufferflies |
| err := curGroup.PatchSettings(group) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !krgm.writeRole.AllowsMetadataWrite() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Do we need to check AllowsMetadataWrite first and PatchSettings secondly?
There was a problem hiding this comment.
PatchSettings is an in-memory operation, I don't think it needs to be prohibited?
2 participants
What problem does this PR solve?
Issue Number: ref #9737
To migrate RM metadata writes to PD in a controlled way, we need explicit and testable write ownership in the manager layer first. Without write-role gates, later cutover steps will mix metadata/state persistence responsibilities and are easy to regress.
What is changed and how does it work?
ResourceGroupWriteRolewith three modes:LegacyAll,PDMetaOnly,RMTokenOnly.AllowsMetadataWrite,AllowsStateWrite) anderrMetadataWriteDisabled.NewManager, defaulting toLegacyAll(no behavior change by default).Add/Modify/DeleteResourceGroup,UpdateControllerConfigItem,SetKeyspaceServiceLimit).SetKeyspaceServiceLimiterror return.Check List
Tests
Code changes
Related changes
pingcap/docs/pingcap/docs-cn: Nonepingcap/tiup: NoneRelease note
Summary by CodeRabbit
New Features
Bug Fixes